Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Handle providerID changing in cluster state #166

Merged
merged 2 commits into from
Jan 18, 2023
Merged

fix: Handle providerID changing in cluster state #166

merged 2 commits into from
Jan 18, 2023

Conversation

jonathan-innis
Copy link
Member

@jonathan-innis jonathan-innis commented Jan 18, 2023 •
edited
Loading

Fixes #

Description

  • Handle provider id changing in cluster state
  • Fixes a potential memory leak if the providerID changes and cluster state never cleans up the old providerID

How was this change tested?

make presubmit

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jonathan-innis jonathan-innis requested a review from a team as a code owner January 18, 2023 18:19
@jonathan-innis jonathan-innis force-pushed the fix-provider-id-change branch 2 times, most recently from d8e85ae to d9654b5 Compare January 18, 2023 18:20
@jonathan-innis jonathan-innis enabled auto-merge (squash) January 18, 2023 18:21
@jonathan-innis jonathan-innis changed the title fix: Handle provider id changing in cluster state fix: Handle providerID changing in cluster state Jan 18, 2023
@coveralls
Copy link

coveralls commented Jan 18, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 3952563037

  • 18 of 18 (100.0%) changed or added relevant lines in 1 file are covered.
  • 17 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.1%) to 77.117%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/deprovisioning/controller.go 3 70.33%
pkg/controllers/node/controller.go 5 72.73%
pkg/controllers/deprovisioning/events/events.go 9 80.0%
Totals Coverage Status
Change from base Build 3952112398: 0.1%
Covered Lines: 5857
Relevant Lines: 7595
💛 - Coveralls

tzneal
tzneal previously approved these changes Jan 18, 2023
View reviewed changes
pkg/apis/v1alpha5/labels.go Show resolved Hide resolved
pkg/controllers/state/cluster.go Outdated Show resolved Hide resolved
tzneal
tzneal approved these changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@tzneal tzneal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jonathan-innis jonathan-innis merged commit c38b8b0 into kubernetes-sigs:main Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tzneal tzneal tzneal approved these changes

@spring1843 spring1843 Awaiting requested review from spring1843 spring1843 was automatically assigned from aws/karpenter

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jonathan-innis @coveralls @tzneal